Skip to content

Conversation

Azoy
Copy link
Contributor

@Azoy Azoy commented Jun 8, 2019

This is just an initial implementation for the demangle function that we're discussing over here. Documentation might need some love.

Tests should fail right now because it's dependent on #25308 being resolved first.

cc: @jckarter

@compnerd
Copy link
Member

CC: @mikeash

@Azoy Azoy force-pushed the demangle-me branch 3 times, most recently from 302a396 to 84a4010 Compare July 8, 2019 21:44
Copy link
Contributor

@benrimmington benrimmington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a possible bug in the swift_demangle implementation, assuming that the mangledName doesn't need to be null-terminated.

-  if (!Demangle::isSwiftSymbol(mangledName))
+  if (!Demangle::isSwiftSymbol(StringRef(mangledName, mangledNameLength)))

The comments in Demangle.h and Demangle.cpp could be updated. For example:

-// NB: This function is not used directly in the Swift codebase, but is
+// NB: This function is used in the Swift standard library, but is also
 // exported for Xcode support and is used by the sanitizers. Please coordinate
 // before changing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If mangledNameBuffer is always null-terminated then you could change it to mangledName: UnsafePointer<CChar>, so that String arguments can be implicitly converted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have the String argument version, so I'm unsure why supporting a pointer version for implicit conversion is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm suggesting that:

  • either mangledNameBuffer: UnsafeBufferPointer<Int8> doesn't need to be null-terminated;

  • or mangledName: UnsafePointer<Int8> would make the third demangle function unnecessary.

On the other hand, John McCall wants to deprecate the implicit conversions.

By the way, are Int8 or UInt8 elements more suitable for UTF-8 string parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use your suggestion about using StringRef(mangledName, mangledNameLength)) mangledNameBuffer doesn't need to be null terminated. However, we must commit to either the name buffer being null terminated (because we calculate length with nameBuffer.count - 1), or must not be null terminated (nameBuffer.count).

As for Int8 or UInt8, CChar maps to Int8 and the string functions withCString and utf8CString.withUnsafeBufferPointer, etc all have Int8 element types. withUTF8 on the other hand uses UInt8, so I'm unsure. I think going with Int8 is the better call because of the CChar typealias.

Copy link
Collaborator

@xwu xwu Jul 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CChar is either signed or unsigned, depending on the platform; it reflects the signedness of the C char type.

newline

remove ?

remove user label prefixes

Add swift_demangle shim in SwiftShims

INTERNAL

Update based on forum comments

Update tests

Address some feedback
@Azoy
Copy link
Contributor Author

Azoy commented Aug 24, 2019

I'm going to close this PR and open a new one once we've discussed more about this on the forums.

@Azoy Azoy closed this Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants